Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make beforeunload not affect 'salvageable' & fire unload event only if document is no longer salvageable #5889

Merged
merged 3 commits into from
Oct 12, 2020
Merged

Make beforeunload not affect 'salvageable' & fire unload event only if document is no longer salvageable #5889

merged 3 commits into from
Oct 12, 2020

Conversation

rakina
Copy link
Member

@rakina rakina commented Sep 3, 2020 •
edited by pr-preview bot
Loading

As discussed in #5748 (comment), the existence of the beforeunload event listener should not affect a Document's salvageable bit. Also, if the user agent decides that it will keep a Document alive in a session history entry (i.e. preserving it in the back-forward cache), it should not dispatch the unload event.

  • At least two implementers are interested (and none opposed):
    • Chrome
    • Firefox
  • Tests are written and can be reviewed and commented upon at:
    • N/A ?
  • Implementation bugs are filed:
    • Chrome: Already implemented (beforeunload will be fired, unload will not be fired when page gets into bfcache)
    • Safari: Already implemented (beforeunload will be fired, unload will not be fired when page gets into bfcache)
    • Firefox: Already implemented ((beforeunload & unload will make page ineligible for bfcache, which this allows too)

/browsing-the-web.html ( diff )

@rakina rakina marked this pull request as ready for review September 3, 2020 10:52
@rakina rakina mentioned this pull request Sep 3, 2020
Closed
@domenic domenic mentioned this pull request Sep 3, 2020
Open
domenic
domenic approved these changes Sep 3, 2020
View reviewed changes
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! @annevk, can you do a double-check? Maybe also brainstorm if this is testable?

I'll be out next week, so please go ahead and merge without me.

annevk
annevk reviewed Sep 7, 2020
View reviewed changes
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! @smaug---- could you maybe review this as well?

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
annevk
annevk reviewed Sep 8, 2020
View reviewed changes
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@rakina rakina changed the title Make beforeunload not affect 'salvageable' & firing unload event optional Make beforeunload not affect 'salvageable' & fire unload event only if document is no longer salvageable Sep 11, 2020
@rakina
Copy link
Member Author

rakina commented Sep 16, 2020

Waiting for @smaug----'s review but here's the commit message (let me know if it needs more work):

Make beforeunload not affect 'salvageable' & fire unload event only if document is no longer salvageable

The existence of the beforeunload or unload event listener should not
affect a Document's salvageable state. Furthermore, the user agent
should be able to decide whether a Document should stay salvageable
or not after running "unload a Document" on it. If the Document stays
salvageable, the user agent should not fire the unload event on the
Document during the "unload a Document" steps.

@rakina
Copy link
Member Author

rakina commented Oct 5, 2020

Pinging @smaug---- for review here. Maybe @cdumez wants to review this also, since this is updating the spec to allow Safari's behavior. (can't add @cdumez directly as a reviewer somehow)

@rakina rakina mentioned this pull request Oct 5, 2020
Open
@smaug----
Copy link

The commit message is confusing
"Make beforeunload not affect 'salvageable' & fire unload event only if document is no longer salvageable"
What is beforeunload here? beforeunload listener? Do unload event listener still affect to salvageable? The latter part of the message hints no.

smaug----
smaug---- approved these changes Oct 9, 2020
View reviewed changes
Copy link

@smaug---- smaug---- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable. Super scary and regression prone of course to not care about beforeunload or unload event listeners, but if other browsers have succeeded with that approach...
And beforeunload does become even more weird now that it may fire multiple times.
But not sure what else could be done.

@domenic
Copy link
Member

domenic commented Oct 9, 2020

Here is my draft commit message:

Change beforeunload, unload, and bfcache interaction

Per https://github.com/whatwg/html/issues/5748#issuecomment-685273796,
removes the requirement that beforeunload event listeners being fired
cause a document to become unsalveagable (i.e., ineligible for bfcache).
Instead, unsalveagability is left more explicitly up to the user agent.

Additionally, changes navigation to no longer dispatch unload events for
documents which are kept alive in the session history (bfcache).

Part of #5748.

I was about to merge, however, I'm unsure whether @rakina wants to finish up the comment thread at #5889 (comment) in this PR or a separate one. Also, I'm unsure whether this closes #5748, or just helps with it. Thoughts, @rakina?

pull bot pushed a commit to Alan-love/chromium that referenced this pull request Oct 10, 2020
@rakina
Don't dispatch unload if we've dispatched pagehide with persisted == …
67d49f2 
…true

If we've dispatched the 'pagehide' event with the 'persisted' property
set to true, we should not dispatch the unload event after that. This is
because the 'persisted' property is set to the 'salvageable' status of
the page, which also determines whether the unload event is fired.

If 'salvageable' is true, 'persisted' will be true and 'unload' won't
be dispatched. If 'salvageable' is false, 'persisted' will be false
and 'unload' should be dispatched.

Relevant spec PR: whatwg/html#5889
More context: https://groups.google.com/a/google.com/g/chrome-bfcache/c/L-ZreZDY4n0/m/jna_jQJkCQAJ

Bug: 1110744
Change-Id: I54b44cfdcb6c2922ca57071d695df6c4c2d77d77
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2455510
Commit-Queue: Rakina Zata Amni <[email protected]>
Reviewed-by: Kentaro Hara <[email protected]>
Reviewed-by: Alexander Timin <[email protected]>
Cr-Commit-Position: refs/heads/master@{#815898}
@rakina
Copy link
Member Author

rakina commented Oct 12, 2020

Thanks @domenic and @smaug----! The draft commit message LGTM, and I've removed the "fired unload" flag as well now per #5889 (comment). This should close #5748, as I think the remaining issues related to this part of the spec are tracked in #6026 and #5879.

@rakina
Make beforeunload not affect 'salvageable' & firing unload event opti…
ddb52fd 
…onal

Currently firing beforeunload on a document will set its 'salvageable'
bit to false, which is not true in practice in some user agents.

Also, firing the 'unload' event should be optional if the user agent
decides that it will keep the document alive in a session history entry,
to be used later.
@rakina
Set salvageable even when there's no unload listeners
7ac37b5
@rakina
Remove 'fired unload' flag
b8fcd45
@domenic domenic merged commit b3b7add into whatwg:master Oct 12, 2020
This was referenced Oct 12, 2020
Closed
Closed
Open
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
@rakina @copybara-github
Don't dispatch unload if we've dispatched pagehide with persisted == …
0a6a98d 
…true

If we've dispatched the 'pagehide' event with the 'persisted' property
set to true, we should not dispatch the unload event after that. This is
because the 'persisted' property is set to the 'salvageable' status of
the page, which also determines whether the unload event is fired.

If 'salvageable' is true, 'persisted' will be true and 'unload' won't
be dispatched. If 'salvageable' is false, 'persisted' will be false
and 'unload' should be dispatched.

Relevant spec PR: whatwg/html#5889
More context: https://groups.google.com/a/google.com/g/chrome-bfcache/c/L-ZreZDY4n0/m/jna_jQJkCQAJ

Bug: 1110744
Change-Id: I54b44cfdcb6c2922ca57071d695df6c4c2d77d77
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2455510
Commit-Queue: Rakina Zata Amni <[email protected]>
Reviewed-by: Kentaro Hara <[email protected]>
Reviewed-by: Alexander Timin <[email protected]>
Cr-Commit-Position: refs/heads/master@{#815898}
GitOrigin-RevId: 67d49f2adf8c7b9dbe6166b152a7901addb02d5c
@tunetheweb tunetheweb mentioned this pull request Dec 5, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@annevk annevk annevk left review comments

@domenic domenic domenic approved these changes

@smaug---- smaug---- smaug---- approved these changes

Assignees
No one assigned
Labels
normative change topic: navigation
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rakina @smaug---- @domenic @annevk